Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Direct dispatch to fleet or robot #1004

Merged
merged 23 commits into from
Sep 18, 2024
Merged

Conversation

aaronchongth
Copy link
Member

@aaronchongth aaronchongth commented Aug 30, 2024

What's new

image
image

fleet-robot-2024-08-30_14.11.35.mp4

Self-checks

  • I have prototyped this new feature (if necessary) on Figma
  • I'm familiar with and follow this Typescript guideline
  • I added unit-tests for new components
  • I tried testing edge cases
  • I tested the behavior of the components that interact with the backend, with an e2e test

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Base automatically changed from feat/labels-priority-schedule to main September 3, 2024 01:20
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
packages/dashboard/src/components/appbar.tsx Outdated Show resolved Hide resolved
packages/dashboard/src/components/appbar.tsx Outdated Show resolved Hide resolved
packages/dashboard/src/components/appbar.tsx Outdated Show resolved Hide resolved
packages/dashboard/src/components/tasks/task-schedule.tsx Outdated Show resolved Hide resolved
packages/dashboard/src/components/tasks/task-schedule.tsx Outdated Show resolved Hide resolved
packages/react-components/lib/tasks/create-task.tsx Outdated Show resolved Hide resolved
packages/react-components/lib/tasks/create-task.tsx Outdated Show resolved Hide resolved
packages/react-components/lib/tasks/create-task.tsx Outdated Show resolved Hide resolved
packages/react-components/lib/tasks/create-task.tsx Outdated Show resolved Hide resolved
…scheduled fleet task

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…d comments

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@@ -331,7 +350,8 @@ export const TaskSchedule = () => {
setEventScope(EventScopes.CURRENT);
AppEvents.refreshTaskSchedule.next();
}}
submitTasks={submitTasks}
onDispatchTask={dispatchTaskCallback}
onScheduleTask={editScheduledTaskCallback}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the same code for both this and the appbar? The way it is now has quite abit of code smell, onScheduleTask has different meaning depending on the context, we need to decouple it, my ideas are either

  1. Have 2 different callback explicitly, for create and edit, then both here and appbar can use the same code.
  2. Use different components, i.e. have a EditTask component, it can use CreateTaskForm internally but with some differences in behavior and text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Comment on lines 308 to 309
onScheduleTask?(task: TaskRequest, schedule: Schedule): Promise<void>;
onEditScheduleTask?(task: TaskRequest, schedule: Schedule): Promise<void>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add docs for these 2 props and schedule. iiuc, only onEditScheduleTask is used in "edit" mode, which is controlled by the schedule prop and onScheduleTask is used otherwise. They are mutually exclusive and will never be used together.

Better would be to refactor this to use the micro app hooks, the task form should be able to handle all the api calls itself and all these callbacks should be able to be replaced with a single onClose callback. But I leave it up to you if you want to do the refactor now.

Copy link
Member Author

@aaronchongth aaronchongth Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they technically can be used together, just that we choose not to provide a callback in whichever place we are creating a task form, i.e. the edit schedule component can also provide onScheduleTask but because of the function itself, we choose not to provide it.

yeah using hooks will be ideal, I will help look into it when we finally merge react components and dashboard

edit: more specifically, buttons will be rendered, based on the availability of these callback functions, if both are provided, both buttons will be rendered.

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth merged commit 5d670b5 into main Sep 18, 2024
3 checks passed
@aaronchongth aaronchongth deleted the feature/fleet-robot-selection branch September 18, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[rmf-dashboard] Support for /robot_task for task form
2 participants